[No QA] Replace contributorValidationGate with isAuthorizedContributor action#91485
Conversation
Centralize gate logic and Contributor+ team membership ahead of linked issue/PR checks. Co-authored-by: Cursor <cursoragent@cursor.com>
Bundle contributor authorization into a node24 action with ncc and rebuild actions that embed CONST after adding contributor-plus team slug. Co-authored-by: Cursor <cursoragent@cursor.com>
…or action Colocate authorization checks in caller workflows and authorize Contributor+ team members. Co-authored-by: Cursor <cursoragent@cursor.com>
Local actions require the repository on the runner per App workflow guidelines. Co-authored-by: Cursor <cursoragent@cursor.com>
Match @octokit/request-error RequestErrorOptions shape for typecheck. Co-authored-by: Cursor <cursoragent@cursor.com>
Remove ContributorAuthorization lib; export helpers from the action entry for unit tests. Drop CONTRIBUTOR_PLUS_TEAM_SLUG from CONST. Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Dedupe link parsing and error logging; add isMatchingLogin helper. Co-authored-by: Cursor <cursoragent@cursor.com>
PR template issue links are visible and placeholders in comments do not match issue IDs. Co-authored-by: Cursor <cursoragent@cursor.com>
Remove loginsMatch and isMatchingLogin helpers. Co-authored-by: Cursor <cursoragent@cursor.com>
Being the author or reviewer of some unrelated PR is not a meaningful authorization signal; contributors with merged PRs already have CONTRIBUTOR association which is checked first. Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
864e3a2 to
dbbacf6
Compare
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
|
@stitesExpensify Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c0b1831a2b
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| const prBody = pr.body ?? ''; | ||
| const isAuthorized = await isAuthorizedViaLinkedIssues(prBody, prAuthor); |
There was a problem hiding this comment.
Restore linked-PR authorization checks for contributors
This change regresses the contributor gate by removing the linked-PR authorization path: isAuthorizedContributor() now only evaluates isAuthorizedViaLinkedIssues(...) after loading the PR body, so external authors who are valid via a linked PR (author/reviewer/requested reviewer) are incorrectly treated as unauthorized. That breaks the prior behavior used by validateContributorPR.yml/claude-review.yml and can cause legitimate contributor PRs to be auto-closed or skipped from downstream workflows.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
I intentionally removed this. Here's a snippet from my conversation with Claude:
Rory: What does this function imply? That a contributor can open a pull request because they have another one open? Do we need that for something, or could we just remove it entirely?
...
Claude: The logic implies three separate authorization paths via linked PRs:
- You authored the linked PR (you've already contributed work)
- You reviewed the linked PR (you've already done review work)
- You were requested to review the linked PR (an internal team member nominated you)
The third case (#3 — requested reviewer) is the most defensible: it's essentially "an Expensify employee explicitly invited this person to be involved." But cases #1 and #2 are weak — having authored or reviewed some other PR doesn't really mean you're authorized to open this one.
More importantly, if someone had a PR merged (case #1), they'd already have the CONTRIBUTOR association, which is checked first and would short-circuit before ever reaching this function. So the "you authored a linked PR" path only fires for unmerged PRs — which is even less meaningful as an authorization signal.
The linked issues check is the principled one: being assigned to an issue means someone explicitly tasked you with the work. The linked PR check has no equivalent clear intent.
My recommendation: Remove isAuthorizedViaLinkedPullRequests entirely. The only case worth preserving might be "requested reviewer," but that's a very unusual path and I'd want to know if it was ever actually used before keeping the complexity. Want me to delete the function and update the tests?
…ontributor Co-authored-by: Cursor <cursoragent@cursor.com> # Conflicts: # .github/workflows/authorChecklist.yml # .github/workflows/claude-review.yml
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppiOS: mWeb SafariMacOS: Chrome / Safari |
There was a problem hiding this comment.
Pull request overview
This PR replaces the reusable contributorValidationGate workflow with a bundled TypeScript GitHub Action (isAuthorizedContributor) to centralize and reuse external-contributor authorization checks without spinning up a separate runner per caller.
Changes:
- Added a new Node 24 JavaScript action (
isAuthorizedContributor) that determines whether a PR author is authorized (association, Contributor+ team membership, linked issue assignee). - Updated gating workflows (
claude-review,authorChecklist,cla,validateContributorPR) to checkout first and then call the local action directly. - Removed the
contributorValidationGate.ymlreusable workflow and wired the new action into the action build script and unit tests.
Reviewed changes
Copilot reviewed 9 out of 10 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
tests/unit/isAuthorizedContributorTest.ts |
Adds unit coverage for the new authorization logic. |
.github/actions/javascript/isAuthorizedContributor/action.yml |
Defines the new Node 24 GitHub Action interface (inputs/outputs). |
.github/actions/javascript/isAuthorizedContributor/isAuthorizedContributor.ts |
Implements authorization logic (association, Contributor+, linked issues) and sets action output. |
.github/actions/javascript/isAuthorizedContributor/index.js |
Bundled ncc output used by the action runner. |
.github/scripts/buildActions.sh |
Includes the new action entrypoint in the build list. |
.github/workflows/validateContributorPR.yml |
Switches from reusable workflow to local action and gates close/lock behavior on the action output. |
.github/workflows/contributorValidationGate.yml |
Removes the no-longer-needed reusable gate workflow. |
.github/workflows/claude-review.yml |
Inlines authorization gating via the local action and centralizes “authorized” branching. |
.github/workflows/cla.yml |
Replaces reusable gate workflow usage with the local action and exposes IS_AUTHORIZED as a job output. |
.github/workflows/authorChecklist.yml |
Runs the local gate action and only executes the checklist step when authorized. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -0,0 +1,25 @@ | |||
| name: 'Is Authorized Contributor' | |||
| description: 'Checks whether a PR author is authorized to contribute (org member, Contributor+, or linked issue/PR)' | |||
Codecov Report✅ All modified and coverable lines are covered by tests. |
|
@mountiny updated and ready for another review |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚀 Deployed to staging by https://github.com/stitesExpensify in version: 9.3.83-0 🚀
Bundle Size Analysis (Sentry): |
|
🚀 Deployed to production by https://github.com/mountiny in version: 9.3.83-3 🚀
|
Explanation of Change
Background
contributorValidationGate.ymlhad four callers:What this PR changes
contributorValidationGatereusable workflow (which unnecessarily spins up a new runner per caller) with a TypeScriptisAuthorizedContributorGitHub action built via ncc.contributor-plusorg team (viaOS_BOTIFY_TOKEN), in addition to existing association, linked-issue assignee, and linked-PR reviewer checks. This is safe for all current use-cases but worht noting - Contributor+ will be able to open pull requests and have them auto-reviewed by Claude.Fixed Issues
side-quest for #90214
Tests
Automated tests only.
Offline tests
N/A – GitHub Actions only.
QA Steps
N/A – GitHub Actions only.
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectioncanBeMissingparam foruseOnyxtoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))npm run compress-svg)Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
N/A